Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable more memory regions on the QVM #873

Merged
merged 14 commits into from
Aug 23, 2019
Merged

Enable more memory regions on the QVM #873

merged 14 commits into from
Aug 23, 2019

Conversation

ecpeterson
Copy link
Contributor

The QVM returns the full classical memory contents with each run. This PR exposes this functionality at the QuantumComputer.read_memory level, while retaining the restriction that the QPU only permits ro memory reads.

One might consider toggling this behavior when in pursuit of QPU realism, which I know is a broad feature under discussion—but I'd like this feature to be exposed to users who care to employ it.

@ecpeterson
Copy link
Contributor Author

@karalekas how do you feel about merging this? kyle has some interest in a cousin PR, #874, which has some friction with this one. it would be good to either get in this first or decide to drop it.

@joachimneu
Copy link

joachimneu commented May 29, 2019

Is it possible to specify which memory regions should be retrieved from the QVM? As long as the parameter classical_addresses of _qvm_run() in https://github.com/rigetti/pyquil/pull/873/files#diff-a58f887d22e0687c320a6c4ddf367130L476 is set using get_classical_addresses_from_program, will the QVM still return memory regions other than "ro"?

(Thanks for your PR, I needed to read classical memory beyond "ro" as well -- your work was a great starting point!)

@ecpeterson
Copy link
Contributor Author

My understanding is that the classical_addresses parameter (or, rather, the analogue of that parameter that actually appears in the QVM payload; I'm not sure if the python parameter gets substantially mangled before making it to the payload) is a dictionary mapping memory names to either a list of indices that the QVM will report on or to True, a synonym for 'all indices'. If you omit a key from this dictionary, that entire memory region won't be reported to the user after a run. If you convert True to some other list of addresses, those addresses not appearing in the list won't be reported to the user after a run.

I'm pretty sure that get_classical_addresses_from_program is set up to have the QVM report everything, and that pyQuil master (but not on this branch) is set up to discard all regions but the ro region.

pyquil/api/_qpu.py Outdated Show resolved Hide resolved
Copy link
Contributor

@appleby appleby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you dropped a return? Otherwise LGTM

pyquil/api/_qam.py Outdated Show resolved Hide resolved
pyquil/api/_qpu.py Outdated Show resolved Hide resolved
pyquil/api/_qpu.py Outdated Show resolved Hide resolved
pyquil/api/_qvm.py Outdated Show resolved Hide resolved
pyquil/pyqvm.py Outdated Show resolved Hide resolved
@karalekas karalekas added this to the v2.11 milestone Jul 29, 2019
Copy link
Contributor

@karalekas karalekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like there are still some open comments from @appleby

@karalekas
Copy link
Contributor

Also, I deleted the changes.rst entry when resolving the merge conflict (on purpose). Please add the entry back, but now to CHANGELOG.md:

  • Add the ability to query for other memory regions after both QPU and QVM runs.
    This removes a previously unnecessary restriction on the QVM, although ro
    remains the only QPU-writeable memory region during Quil execution (@ecpeterson, Enable more memory regions on the QVM #873).

@ecpeterson
Copy link
Contributor Author

ecpeterson commented Aug 23, 2019

OK: I replied to the sets of comments from @karalekas and from @appleby .

Copy link
Contributor

@appleby appleby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@karalekas karalekas changed the title Feature: more memory regions on QVM Enable more memory regions on the QVM Aug 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants